[MM-228] Add feature to update custom status and status of the user during meeting #359

raghavaggarwal2308 · 2024-03-04T13:39:42Z

Recreating this PR with the changes of #355 because of a lot of merge conflicts on that PR

Summary

Added feature to update custom status of the user during meeting
Added feature to set status of the user from the options provided in the dropdown during the meeting

Ticket Link

#228

Screenshots

What to test?

Custom status gets updated during a meeting.
User status gets updated to the selected value during a meeting.
No change in user status during overlapping meetings or when there are no attendees.
No change of user custom status during overlapping meetings, when there are no attendees, or when there is already a custom status set.

How to test?

Create a meeting in the MS Calendar.
Run slash command /mscalendar settings, select an option from the Update Status setting, and check the updated status on the Mattermost.
Run slash command /mscalendar settings, set the Set Custom Status setting to yes, and check the updated custom status on the Mattermost.

…uring meeting

…move extra condition

codecov-commenter · 2024-03-04T13:47:41Z

Codecov Report

Attention: Patch coverage is 46.55172% with 62 lines in your changes are missing coverage. Please review.

Project coverage is 21.92%. Comparing base (7f1bbcd) to head (642b212).

Files	Patch %	Lines
calendar/engine/availability.go	57.44%	30 Missing and 10 partials ⚠️
calendar/engine/welcome_flow.go	0.00%	11 Missing ⚠️
calendar/engine/settings.go	0.00%	9 Missing ⚠️
calendar/engine/welcomer.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
+ Coverage   20.94%   21.92%   +0.98%     
==========================================
  Files          67       67              
  Lines        3442     3525      +83     
==========================================
+ Hits          721      773      +52     
- Misses       2628     2652      +24     
- Partials       93      100       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…event

hanzei · 2024-03-05T08:21:08Z

I'm not too knowldeage with this plugin. @mickmister @fmartingr would you mind reviewing the PR?

fmartingr

This seems the same as I reviewed back in #355 👍

mickmister · 2024-05-21T15:21:09Z

@AayushChaudhary0001 Can you please take a look at this PR when you have the chance? Thanks

mickmister

Awesome work @raghavaggarwal2308 👍

I have some comments for discussion

calendar/engine/availability.go

mickmister · 2024-05-21T16:22:16Z

calendar/engine/availability.go

+	// Not setting custom status for events without attendees since those are unlikely to be meetings.
+	if len(events[0].Attendees) < 1 {
+		return "No attendee present, not setting custom status", isStatusChanged, nil
+	}


What if a meeting other than the first one has attendees?

This flow is updated now. We consider other meetings as well now.

calendar/engine/availability.go

mickmister · 2024-05-21T16:43:37Z

calendar/engine/welcome_flow.go

@@ -56,15 +56,18 @@ func (wf *WelcomeFlow) FlowDone(userID string) {

 func (wf *WelcomeFlow) makeSteps() {
 	steps := []flow.Step{
+		&flow.EmptyStep{
+			Title:   "Update Status",
+			Message: "You can update your status to \"Away\" or \"Do not disturb\" when you're in a meeting by typing `/mscalendar settings`.",


This original wording makes me think it's telling me to run /mscalendar settings while I'm in a meeting

Suggested change

Message: "You can update your status to \"Away\" or \"Do not disturb\" when you're in a meeting by typing `/mscalendar settings`.",

Message: "You can type `/mscalendar settings` to configure the plugin to update your status to \"Away\" or \"Do not disturb\" when you're in a meeting.",

Can we use ProviderConfig.Command here instead of a fixed /mscalendar?

@fmartingr I wonder if this is something we can configure the linter to catch?

mickmister · 2024-05-21T16:45:14Z

calendar/plugin/plugin.go

+			e.Dependencies.Tracker = tracker.New(
+				telemetry.NewTracker(
+					p.telemetryClient,
+					p.API.GetDiagnosticId(),
+					p.API.GetServerVersion(),
+					e.PluginID,
+					e.PluginVersion,
+					config.Provider.TelemetryShortName,
+					telemetry.NewTrackerConfig(p.API.GetConfig()),
+					telemetry.NewLogger(p.API),
+				),


Interesting. So we haven't been tracking telemetry?

There was no else condition present in the block in case e.Dependencies.Tracker is nil. So added the else condition as well.

calendar/store/user_store.go

mickmister · 2024-05-21T16:54:34Z

calendar/engine/availability_test.go

+		"SetCustomStatus enabled but overlapping events": {
+			settings: store.Settings{
+				SetCustomStatus: true,
+			},
+			runAssertions: func(deps *Dependencies, client remote.Client) {
+				c, papi, _, _, r := client.(*mock_remote.MockClient), deps.PluginAPI.(*mock_plugin_api.MockPluginAPI), deps.Poster.(*mock_bot.MockPoster), deps.Store.(*mock_store.MockStore), deps.Remote.(*mock_remote.MockRemote)
+				moment := time.Now().UTC()


What if I have a meeting in the next hour, and in the following hour after that I have two meetings that overlap. Will this make it so the first meeting (which in this case does not overlap) doesn't trigger a status update? Can we have a test made for this?

This case is updated now and we are handling overlapping events now.

calendar/engine/availability_test.go

mickmister · 2024-05-22T17:55:46Z

From the discussion #228 (comment), it seems we should still change the user's status when there are overlapping meetings. We want to emulate real life as much as possible. If there is an event where I'm a (confirmed?) attendee, and there are other attendees, then the user's status should change

mickmister · 2024-06-10T16:16:59Z

Hi @raghavaggarwal2308 just checking in. Do you or another dev plan to continue work on this PR?

raghavaggarwal2308 · 2024-06-11T05:46:43Z

Hi @raghavaggarwal2308 just checking in. Do you or another dev plan to continue work on this PR?

@mickmister Yeah, we will pick this up asap. Apologies for the delay.

…st cases

ayusht2810 · 2024-07-04T12:46:29Z

@mickmister Fixed the review comment. Please re-review

mickmister · 2024-07-10T00:36:31Z

calendar/engine/availability.go

@@ -110,7 +110,7 @@ func (m *mscalendar) retrieveUsersToSync(userIndex store.UserIndex, syncJobSumma
 		}

 		// If user does not have the proper features enabled, just go to the next one
-		if !(user.Settings.UpdateStatusFromOptions != NotSetStatusOption || user.Settings.ReceiveReminders || user.Settings.SetCustomStatus) {
+		if !(user.IsConfiguredForStatusUpdates() || user.IsConfiguredForCustomStatusUpdates() || user.Settings.ReceiveReminders) {


I think I would expect these methods to be defined on user.Settings, though I don't think it matters. This current way allows for less typing throughout the usages of the method

calendar/engine/availability.go

mickmister · 2024-07-10T00:40:42Z

calendar/engine/availability.go

 	for i := 1; i < len(events); i++ {
-		if events[i-1].End.Time().UnixMicro() > events[i].Start.Time().UnixMicro() {
-			return true
+		if (events[idx].End.Time().UnixMicro() >= events[i].Start.Time().UnixMicro()) || (events[idx].End.Time().Sub(events[idx].Start.Time()) <= StatusSyncJobInterval && events[i].Start.Time().Sub(events[idx].End.Time()) <= StatusSyncJobInterval) {
+			events[idx].End = events[i].End
+		} else {
+			idx++
+			events[idx] = events[i]
 		}
 	}

-	return false
+	return events[0 : idx+1]


It would be good to put the body of this loop in its own function, and have it unit tested to show/document how it works

@mickmister Added a comment to it. I don't think we need to separately test the function as it is already covered by its parent function.

mickmister · 2024-07-10T00:43:03Z

calendar/engine/availability_test.go

@@ -856,3 +1025,161 @@ func makeStatusSyncTestEnv(ctrl *gomock.Controller) (Env, remote.Client) {

 	return env, mockClient
 }
+
+func TestGetMergedEvents(t *testing.T) {


I see the function is tested here 👍

mickmister · 2024-07-10T00:50:59Z

Hi @AayushChaudhary0001, can you please review this when you have a chance?

ayusht2810 · 2024-07-10T11:59:26Z

@mickmister Fixed the review fixes. Please re-review.

arush-vashishtha · 2024-07-15T07:40:43Z

@raghavaggarwal2308 I found two issue while testing this PR

Issue 01: Wrong text is shown for the status change.

Steps to reproduce:

Enable the MS calendar plugin
Open the ms calendar settings for Mattermost server via slash command
Then set 'Setting: Get Confirmation' to Yes
Also set 'Setting: Set Custom Status' to Yes
Now schedule a meeting from outlook calendar
Check the text shown for status change message.

Expected: Appropriate and accurate text should be shown.
Actual: Wrong text is shown for the status change.

SS for the reference:

Issue 02: Custom status is not getting updated properly

Steps to reproduce:

Enable the MS calendar plugin
Create two overlapping meeting from outlook calendar like - one from 10:00 am to 10:15 am and second from 10:15 am to 10:30 am
Check the custom status for second meeting

Expected: No delay should be there in updating the custom status of the second meeting
Actual: Around 4 min delay is found for the updation of custom status for the second meeting

wiggin77 · 2024-08-08T22:08:09Z

@raghavaggarwal2308 I didn't realize this PR was needed for the custom status in Google Calendar plugin as well. We have a customer waiting on the GCal part. We'll need to test and merge this despite wanting to avoid big changes in MSCal, unless there is a better solution.

raghavaggarwal2308 · 2024-08-09T10:34:36Z

@wiggin77 Sure, we will work on this PR on priority. Also, the issue in mscalendar plugin has been fixed now. We have also got the confirmation from the customer who was facing the issue earliar.

…lendar into MM-188-fix

raghavaggarwal2308 · 2024-08-13T08:34:55Z

@AayushChaudhary0001 @arush-vashishtha The issues reported above are very rare edge cases. These are happening because our job to update status is running in every 5 mins. So, when two meetings are very close there might be some inconsistencies rarely. I think we should create a new issue for this particular case and spend time on this there. As this is out of scope for this PR.

AayushChaudhary0001 · 2024-08-13T09:49:02Z

Approved this PR because the bug found are possibly out of the scope of this PR, these might be due to the time interval for updating the status by the job. Rest everything looks working fine in this PR(updating the custom status).
Created a new issue regarding for the above #393

ayusht2810 and others added 6 commits March 4, 2024 17:37

[MM-188] Add feature to update custom status and status of the user d…

ed87b58

…uring meeting

[MM-188] Add test case for custom status change

b696601

Fix test cases

d1c271b

[MM-188] Review fixes: Update constant name, add some comments and re…

a9c4531

…move extra condition

[MM-252] Review fixes

10d6649

[MM-250] Review fixes

3c2ed2b

raghavaggarwal2308 requested a review from mickmister as a code owner March 4, 2024 13:39

raghavaggarwal2308 added 2 commits March 4, 2024 19:11

fix link error

56441cc

Fix failing test

8fd8412

raghavaggarwal2308 mentioned this pull request Mar 4, 2024

[MM-188] Add feature to update custom status and status of the user during meeting #355

Closed

[MM-256] Added test cases for back-to-back event and non overlapping …

642b212

…event

raghavaggarwal2308 self-assigned this Mar 5, 2024

raghavaggarwal2308 requested review from fmartingr, hanzei, cwarnermm and AayushChaudhary0001 March 5, 2024 08:10

hanzei removed their request for review March 5, 2024 08:20

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 5, 2024

cwarnermm approved these changes Mar 5, 2024

View reviewed changes

fmartingr approved these changes Mar 11, 2024

View reviewed changes

Merge branch 'master' into MM-188-fix

59f5951

mickmister reviewed May 21, 2024

View reviewed changes

Merge branch 'master' into MM-188-fix

85fe326

[MM-188] Fix review fixes: Use legacy settings, update and add new te…

dbd3e5d

…st cases

ayusht2810 requested a review from wiggin77 as a code owner July 4, 2024 12:19

ayusht2810 and others added 2 commits July 4, 2024 18:06

[MM-188] Update case to filter events on attendee as well

14eddb0

Merge branch 'master' into MM-188-fix

8f503b1

Merge branch 'master' into MM-188-fix

0ad2bb8

raghavaggarwal2308 requested a review from mickmister July 5, 2024 08:05

mickmister approved these changes Jul 10, 2024

View reviewed changes

mickmister removed the 2: Dev Review Requires review by a core committer label Jul 10, 2024

raghavaggarwal2308 and others added 2 commits July 10, 2024 14:04

Merge branch 'master' into MM-188-fix

03b1d25

[MM-188] Add comments to the code

5b08060

wiggin77 and others added 3 commits August 1, 2024 16:13

Merge branch 'master' into MM-188-fix

c7441e0

Merge branch 'master' into MM-188-fix

f287370

Merge branch 'master' into MM-188-fix

b851b21

wiggin77 approved these changes Aug 8, 2024

View reviewed changes

wiggin77 requested a review from ayusht2810 August 8, 2024 22:06

raghavaggarwal2308 added this to the v1.3.0 milestone Aug 9, 2024

Merge branch 'master' of github.com:mattermost/mattermost-plugin-msca…

716ebcc

…lendar into MM-188-fix

raghavaggarwal2308 mentioned this pull request Aug 13, 2024

[MM-675]: Fixed the inconsistency among the different setting message #389

Merged

AayushChaudhary0001 approved these changes Aug 13, 2024

View reviewed changes

raghavaggarwal2308 removed the 3: QA Review Requires review by a QA tester label Aug 13, 2024

raghavaggarwal2308 merged commit 1589e57 into master Aug 13, 2024
6 checks passed

raghavaggarwal2308 deleted the MM-188-fix branch August 13, 2024 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-228] Add feature to update custom status and status of the user during meeting #359

[MM-228] Add feature to update custom status and status of the user during meeting #359

raghavaggarwal2308 commented Mar 4, 2024 •

edited

Loading

codecov-commenter commented Mar 4, 2024 •

edited

Loading

hanzei commented Mar 5, 2024

fmartingr left a comment

mickmister commented May 21, 2024

mickmister left a comment

mickmister May 21, 2024

ayusht2810 Jul 4, 2024

mickmister May 21, 2024

fmartingr Jun 10, 2024

mickmister Jun 10, 2024

mickmister May 21, 2024

ayusht2810 Jul 4, 2024

mickmister May 21, 2024

ayusht2810 Jul 4, 2024

mickmister commented May 22, 2024

mickmister commented Jun 10, 2024

raghavaggarwal2308 commented Jun 11, 2024

ayusht2810 commented Jul 4, 2024

mickmister Jul 10, 2024

mickmister Jul 10, 2024

ayusht2810 Jul 10, 2024

mickmister Jul 10, 2024

mickmister commented Jul 10, 2024

ayusht2810 commented Jul 10, 2024

arush-vashishtha commented Jul 15, 2024

wiggin77 commented Aug 8, 2024

raghavaggarwal2308 commented Aug 9, 2024

raghavaggarwal2308 commented Aug 13, 2024

AayushChaudhary0001 commented Aug 13, 2024

	Message: "You can update your status to \"Away\" or \"Do not disturb\" when you're in a meeting by typing `/mscalendar settings`.",
	Message: "You can type `/mscalendar settings` to configure the plugin to update your status to \"Away\" or \"Do not disturb\" when you're in a meeting.",

[MM-228] Add feature to update custom status and status of the user during meeting #359

[MM-228] Add feature to update custom status and status of the user during meeting #359

Conversation

raghavaggarwal2308 commented Mar 4, 2024 • edited Loading

Summary

Ticket Link

Screenshots

What to test?

How to test?

codecov-commenter commented Mar 4, 2024 • edited Loading

Codecov Report

hanzei commented Mar 5, 2024

fmartingr left a comment

Choose a reason for hiding this comment

mickmister commented May 21, 2024

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister commented May 22, 2024

mickmister commented Jun 10, 2024

raghavaggarwal2308 commented Jun 11, 2024

ayusht2810 commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister commented Jul 10, 2024

ayusht2810 commented Jul 10, 2024

arush-vashishtha commented Jul 15, 2024

wiggin77 commented Aug 8, 2024

raghavaggarwal2308 commented Aug 9, 2024

raghavaggarwal2308 commented Aug 13, 2024

AayushChaudhary0001 commented Aug 13, 2024

raghavaggarwal2308 commented Mar 4, 2024 •

edited

Loading

codecov-commenter commented Mar 4, 2024 •

edited

Loading